-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixed nested ':=' reference assignment fails #6789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
*Followed TODO: by mattdowle from resolution to '2-space indentation Rdatatable#2420' *Added tests for jsub that modify DT by-reference *Added test case for interger vector indexing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6789 +/- ##
=======================================
Coverage 99.06% 99.06%
=======================================
Files 86 86
Lines 16604 16613 +9
=======================================
+ Hits 16449 16458 +9
Misses 155 155 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @AntonNM would you mind merging to current I would usually attempt to do so myself, but your PR is from your fork's |
Co-authored-by: Marco Colombo <[email protected]>
Fixed nested ':=' reference assignment fails (#6768)
By reference assignments (':=') with functions that modified the data.table by reference e.g. (
foo=function(DT){modify(DT);return(1L)},DT[,a:=foo(DT)]) returned a mallformed data.table due to the modification of the targeted named column index ("a") during the j expression evaluation*Completed TODO: by mattdowle from commits to '2-space indentation #2420' to move allocation logic after jsub evaluation
chmatch and setalloccol calls moved below jsub evaluation which fixed name to column index mapping. Warnings and conditions to terminate early remain before the evaluation. selfrefok warning and following branch were split to accommodate this and to avoid duplicate warnings
*Added tests for jsub expressions that modify DT by-reference
Includes the test that initially demonstrated the bug and an additional test case for future regression testing.
*Added test case for integer vector indexing
Removed pre-jsub-eval numerical lhs to name mapping to prevent issues with duplicate name issues.
Closes #6768